Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inference: fix too conservative effects for recursive cycles #54323

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented May 1, 2024

The :terminates effect bit must be conservatively tainted unless recursion cycle has been fully resolved. As for other effects, there's no need to taint them at this moment because they will be tainted as we try to resolve the cycle.

@aviatesk aviatesk added the backport 1.11 Change should be backported to release-1.11 label May 1, 2024
@aviatesk aviatesk requested a review from vtjnash May 1, 2024 13:03
@@ -864,7 +864,8 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize
update_valid_age!(caller, frame.valid_worlds)
isinferred = is_inferred(frame)
edge = isinferred ? mi : nothing
effects = isinferred ? frame.result.ipo_effects : adjust_effects(Effects(), method) # effects are adjusted already within `finish` for ipo_effects
effects = isinferred ? frame.result.ipo_effects : # effects are adjusted already within `finish` for ipo_effects
adjust_effects(effects_for_cycle(frame.ipo_effects), method)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is legal unless we start cycle-converging effects (as we do for exct and rt), which I think is worth a try, but a much bigger change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we probably need to introduce something similar to LimitedAccuracy for effects too?
I agree it's a necessary step but at the same time I don't foresee this PR causing any practical issues as is, so maybe we could merge it now and leave a TODO comment for future consideration?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start cycle-converging effects (as we do for exct and rt)

Actually, I was reflecting on this for unrelated reasons, and realized that this is probably not actually true. For most of these bits, the results are not cycle-converged like exct and rt, but rather are like world-age min/max. The difference is very significant, as world-age-range is converged very cheaply in finish, as all items in the cycle must have exactly the same bits and simply is the intersection of each partial computation (without dependencies that can change other parts of the computation), unlike those others which have unique results which can have cyclic dependencies on each other.

So as long as you add a path to finish that unions together all of the effects (where we do the same for world age range), we should be able to merge and backport this this without the correctness regression.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion Jameson. Do you think 960da62 correctly implements your idea?

I generally agree with this direction, but there seem to be one difference in world-age-range and effects regarding cycles involving Core.Compiler._return_type. In such cycles, the world-age-range should be the same for all frames, but effects do not necessarily have to be, and especially even when there is a cycle in abstract interpretation (via _return_type_tfunc), it doesn't always need to taint the termination of runtime effects (#49119). As a result 960da62 breaks the test cases added in #49119.

@aviatesk
Copy link
Member Author

Added a TODO comment, and I'm planning to merge this soon.

@vtjnash
Copy link
Member

vtjnash commented May 23, 2024

How did you end up addressing the correctness problem Keno noted in his review?

@aviatesk
Copy link
Member Author

I plan to address that in the future. Practically I don't think any issues will arise with this commit as it is.

KristofferC added a commit that referenced this pull request May 28, 2024
Backported PRs:
- [x] #53665 <!-- use afoldl instead of tail recursion for tuples -->
- [x] #53976 <!-- LinearAlgebra: LazyString in interpolated error
messages -->
- [x] #54005 <!-- make `view(::Memory, ::Colon)` produce a Vector -->
- [x] #54010 <!-- Overload `Base.literal_pow` for `AbstractQ` -->
- [x] #54069 <!-- Allow PrecompileTools to see MI's inferred by foreign
abstract interpreters -->
- [x] #53750 <!-- inference correctness: fields and globals can revert
to undef -->
- [x] #53984 <!-- Profile: fix heap snapshot is valid char check -->
- [x] #54102 <!-- Explicitly compute stride in unaliascopy for SubArray
-->
- [x] #54070 <!-- Fix integer overflow in `skip(s::IOBuffer,
typemax(Int64))` -->
- [x] #54013 <!-- Support case-changes to Annotated{String,Char}s -->
- [x] #53941 <!-- Fix writing of AnnotatedChars to AnnotatedIOBuffer -->
- [x] #54137 <!-- Fix typo in docs for `partialsortperm` -->
- [x] #54129 <!-- use correct size when creating output data from an
IOBuffer -->
- [x] #54153 <!-- Fixup IdSet docstring -->
- [x] #54143 <!-- Fix `make install` from tarballs -->
- [x] #54151 <!-- LinearAlgebra: Correct zero element in
`_generic_matvecmul!` for block adj/trans -->
- [x] #54213 <!-- Add `public` statement to `Base.GC` -->
- [x] #54222 <!-- Utilize correct tbaa when emitting stores of unions.
-->
- [x] #54233 <!-- set MAX_OS_WRITE on unix -->
- [x] #54255 <!-- fix `_checked_mul_dims` in the presence of 0s and
overflow. -->
- [x] #54259 <!-- Fix typo in `readuntil` -->
- [x] #54251 <!-- fix typo in gc_mark_memory8 when chunking a large
array -->
- [x] #54276 <!-- Fix solve for complex `Hermitian` with non-vanishing
imaginary part on diagonal -->
- [x] #54248 <!-- ensure package callbacks are invoked when no valid
precompile file exists for an "auto loaded" stdlib -->
- [x] #54308 <!-- Implement eval-able AnnotatedString 2-arg show -->
- [x] #54302 <!-- Specialised substring equality for annotated strs -->
- [x] #54243 <!-- prevent `package_callbacks` to run multiple time for a
single package -->
- [x] #54350 <!-- add a precompile signature to Artifacts code that is
used by JLLs -->
- [x] #54331 <!-- correctly track freed bytes in
jl_genericmemory_to_string -->
- [x] #53509 <!-- revert moving "creating packages" from Pkg.jl -->
- [x] #54335 <!-- When accessing the data pointer for an array, first
decay it to a Derived Pointer -->
- [x] #54239 <!-- Make sure `fieldcount` constant-folds for `Tuple{...}`
-->
- [x] #54288
- [x] #54067
- [x] #53715 <!-- Add read/write specialisation for IOContext{AnnIO} -->
- [x] #54289 <!-- Rework annotation ordering/optimisations -->
- [x] #53815 <!-- create phantom task for GC threads -->
- [x] #54130 <!-- inference: handle `LimitedAccuracy` in
`handle_global_assignment!` -->
- [x] #54428 <!-- Move ConsoleLogging.jl into Base -->
- [x] #54332 <!-- Revert "add unsetindex support to more copyto methods
(#51760)" -->
- [x] #53826 <!-- Make all command-line options documented in all
related files -->
- [x] #54465 <!-- typeintersect: conservative typevar subtitution during
`finish_unionall` -->
- [x] #54514 <!-- typeintersect: followup cleanup for the nothrow path
of type instantiation -->
- [x] #54499 <!-- make `@doc x` work without REPL loaded -->
- [x] #54210 <!-- attach finalizer in `mmap` to the correct object -->
- [x] #54359 <!-- Pkg REPL: cache `pkg_mode` lookup -->

Non-merged PRs with backport label:
- [ ] #54471 <!-- Actually setup jit targets when compiling
packageimages instead of targeting only one -->
- [ ] #54457 <!-- Make `String(::Memory)` copy -->
- [ ] #54323 <!-- inference: fix too conservative effects for recursive
cycles -->
- [ ] #54322 <!-- effects: add new `@consistent_overlay` macro -->
- [ ] #54191 <!-- make `AbstractPipe` public -->
- [ ] #53957 <!-- tweak how filtering is done for what packages should
be precompiled -->
- [ ] #53882 <!-- Warn about cycles in extension precompilation -->
- [ ] #53707 <!-- Make ScopedValue public -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #53286 <!-- Raise an error when using `include_dependency` with
non-existent file or directory -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC mentioned this pull request May 29, 2024
60 tasks
@aviatesk aviatesk force-pushed the avi/issue52938 branch 3 times, most recently from c303fed to dd692a8 Compare June 4, 2024 03:45
@aviatesk
Copy link
Member Author

aviatesk commented Jun 4, 2024

This PR breaks one of the test cases in #49119, but that case might not be very important in reality. That is because if function f calls Core.Compiler.return_type(g, ...), it is likely to actually call g, so if there is a cycle between f and g, :terminates will eventually be tainted anyway.
This PR fixes many other broken test cases and actually solves issues in the GPUCompiler stack, so I want to move forward with it once CI succeeds.

@aviatesk aviatesk force-pushed the avi/issue52938 branch 3 times, most recently from 9f43a32 to d548088 Compare June 4, 2024 15:10
The `:terminates` effect bit must be conservatively tainted unless
recursion cycle has been fully resolved. As for other effects, there's
no need to taint them at this moment because they will be tainted as we
try to resolve the cycle.

- fixes #52938
- xref #51092
@aviatesk aviatesk merged commit 65aeaf6 into master Jun 5, 2024
7 checks passed
@aviatesk aviatesk deleted the avi/issue52938 branch June 5, 2024 01:11
aviatesk added a commit that referenced this pull request Jun 5, 2024
The `:terminates` effect bit must be conservatively tainted unless
recursion cycle has been fully resolved. As for other effects, there's
no need to taint them at this moment because they will be tainted as we
try to resolve the cycle.

- fixes #52938
- xref #51092
@aviatesk aviatesk removed the backport 1.11 Change should be backported to release-1.11 label Jun 5, 2024
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

aviatesk added a commit that referenced this pull request Jun 5, 2024
The `:terminates` effect bit must be conservatively tainted unless
recursion cycle has been fully resolved. As for other effects, there's
no need to taint them at this moment because they will be tainted as we
try to resolve the cycle.

- fixes #52938
- xref #51092
aviatesk added a commit that referenced this pull request Jun 5, 2024
#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, #54323
breaks the test cases from #51092, when backported to
v1.11. On the surface this is because #52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

This commit uses a somewhat hacky approach to resolve this. It
identifies statements involved in the cycle comparing `stmt_edges` to
`callers_in_cycle`, and updates `ssaflags` according to new cycle valid
effects if necessary. This resolves the issue, but ideally, it should be
implemented more safely with the new `edges` format that will be
implemented in the future. For now, this approach should be okay.
aviatesk added a commit that referenced this pull request Jun 6, 2024
#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, #54323
breaks the test cases from #51092, when backported to
v1.11. On the surface this is because #52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

This commit uses a somewhat hacky approach to resolve this. It
identifies statements involved in the cycle comparing `stmt_edges` to
`callers_in_cycle`, and updates `ssaflags` according to new cycle valid
effects if necessary. This resolves the issue, but ideally, it should be
implemented more safely with the new `edges` format that will be
implemented in the future. For now, this approach should be okay.
aviatesk added a commit that referenced this pull request Jun 6, 2024
#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, #54323
breaks the test cases from #51092, when backported to
v1.11. On the surface this is because #52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

This commit uses a somewhat hacky approach to resolve this. It
identifies statements involved in the cycle comparing `stmt_edges` to
`callers_in_cycle`, and updates `ssaflags` according to new cycle valid
effects if necessary. This resolves the issue, but ideally, it should be
implemented more safely with the new `edges` format that will be
implemented in the future. For now, this approach should be okay.
aviatesk added a commit that referenced this pull request Jun 6, 2024
#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, #54323
breaks the test cases from #51092, when backported to
v1.11. On the surface this is because #52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

This commit uses a somewhat hacky approach to resolve this. It
identifies statements involved in the cycle comparing `stmt_edges` to
`callers_in_cycle`, and updates `ssaflags` according to new cycle valid
effects if necessary. This resolves the issue, but ideally, it should be
implemented more safely with the new `edges` format that will be
implemented in the future. For now, this approach should be okay.
aviatesk added a commit that referenced this pull request Jun 6, 2024
#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, #54323
breaks the test cases from #51092, when backported to
v1.11. On the surface this is because #52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

To resolve this issue, this commit traverses `cycle_backedges` to visit
statements involved in the cycle, and updates each `ssaflags` according
to new cycle valid effects if necessary.
aviatesk added a commit that referenced this pull request Jun 7, 2024
…#54689)

#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, #54323
breaks the test cases from #51092, when backported to
v1.11. On the surface this is because #52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

To resolve this issue, this commit traverses `cycle_backedges` to visit
statements involved in the cycle, and updates each `ssaflags` according
to new cycle valid effects if necessary.
aviatesk added a commit that referenced this pull request Jun 7, 2024
…#54689)

#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, #54323
breaks the test cases from #51092, when backported to
v1.11. On the surface this is because #52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

To resolve this issue, this commit traverses `cycle_backedges` to visit
statements involved in the cycle, and updates each `ssaflags` according
to new cycle valid effects if necessary.
aviatesk pushed a commit that referenced this pull request Jun 24, 2024
Follow-on from #54323

@aviatesk I think this should also be disabled on linux? It's been
failing there

On `test i686-linux-gnu`
```
math                                              (7) |         failed at 2024-06-23T13:58:26.792
Test Failed at /cache/build/tester-amdci5-8/julialang/julia-buildkite/julia-2d4ef8a238/share/julia/test/math.jl:1607
  Expression: Core.Compiler.is_foldable(effects)
     Context: effects = (+c,+e,+n,!t,+s,+m,+u,+o)
              T = Float64
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inference regression with external interpreters + recursive functions
3 participants